fix: restrict Substrait physical local file imports - #23172
Conversation
bvolpato
left a comment
bvolpato
left a comment
There was a problem hiding this comment.
Root canonicalization and component-based prefix check look right. I left inline comments on compatibility and security coverage.
| extensions: &HashMap<u32, &String>, | ||
| ) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let options = PhysicalPlanConsumerOptions::default(); | ||
| from_substrait_rel_with_options(ctx, rel, extensions, &options).await |
There was a problem hiding this comment.
from_substrait_rel used to accept these plans, and now it denies them by default. That makes sense for this fix, but it is still a public behavior change. Can we add the api change label and a short 55.0.0 upgrade note pointing callers to from_substrait_rel_with_options?
| } | ||
|
|
||
| fn parse_local_file_path(path: &str) -> Result<PathBuf> { | ||
| match Url::parse(path) { |
There was a problem hiding this comment.
One Windows edge case here: current producer emits bare object-store paths (#20550). An absolute Windows path comes through as C:/..., and Url::parse reads C as the scheme, so this rejects a plan produced by DataFusion itself. Windows CI is disabled right now. Can we check native absolute paths before URL parsing and cover this with a #[cfg(windows)] round-trip test?
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn local_files_must_stay_under_allowed_root() -> Result<()> { |
There was a problem hiding this comment.
Can we add a symlink escape test here? The direct outside-root case proves the prefix check, but not the canonicalization claim. A Unix-only test with a symlink under the allowed root pointing outside would pin down the security property.
2 participants
Which issue does this PR close?
Rationale for this change
Imported Substrait physical plans currently turn
ReadRel.LocalFilesentries into a local filesystem-backed ParquetDataSourceExecwithout requiring the embedding host to approve the referenced local paths. In hosts that accept physical Substrait plans from lower-trust callers, that can let serialized plan input select process-local Parquet files outside intended dataset roots.This change makes local file access during physical plan import explicit instead of ambient.
What changes are included in this PR?
PhysicalPlanConsumerOptionsfor Substrait physical plan import.from_substrait_relas a default-deny wrapper for local file imports.from_substrait_rel_with_optionsso callers can opt in with allowed local file roots.Are these changes tested?
Yes.
cargo fmt --all --checkcargo test -p datafusion-substraitcargo clippy -p datafusion-substrait --all-targets --all-features -- -D warningscargo clippy --all-targets --all-features -- -D warningsAre there any user-facing changes?
Yes. Substrait physical plan consumers that import
ReadRel.LocalFilesnow need to callfrom_substrait_rel_with_optionsand explicitly configure allowed local file roots. The existingfrom_substrait_relAPI no longer imports local files by default.This is an intentional security hardening change. It may require the
api changelabel because it changes behavior for the existing public API and adds a new opt-in API.